xen: Fix event-channel destruction.

Lots of churn to fix a couple of bugs:

 1. If we tried to close an interdomain channel, and the remote domain
    is being destroyed, we immediately return success. But the remote
    domain will place our port in state 'unbound', not 'free'. Hence
    the port is effectively leaked.

 2. If two domains are being destroyed at the same time, and share an
    interdomain channel, the second to attempt the close may
    dereference an invalid domain pointer. Aiee!

Doing som ework to be able to destroy event-channel context much
earlier in domain death was the civilised thing to do.

Signed-off-by: Keir Fraser <keir@xensource.com>

diff --git a/xen/common/domain.c b/xen/common/domain.c
index 13b02d1439741338e4a9854f5fd1adfe9a7796f1..1a8937f1e1c9e3ff47ad91faa8988428a3f141f3 100644 (file)
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -180,6 +180,8 @@ struct domain *domain_create(
     domid_t domid, unsigned int domcr_flags, ssidref_t ssidref)
 {
     struct domain *d, **pd;
+    enum { INIT_evtchn = 1, INIT_gnttab = 2, INIT_acm = 4, INIT_arch = 8 }; 
+    int init_status = 0;
 
     if ( (d = alloc_domain(domid)) == NULL )
         return NULL;
@@ -195,25 +197,29 @@ struct domain *domain_create(
         atomic_inc(&d->pause_count);
 
         if ( evtchn_init(d) != 0 )
-            goto fail1;
+            goto fail;
+        init_status |= INIT_evtchn;
 
         if ( grant_table_create(d) != 0 )
-            goto fail2;
+            goto fail;
+        init_status |= INIT_gnttab;
 
         if ( acm_domain_create(d, ssidref) != 0 )
-            goto fail3;
+            goto fail;
+        init_status |= INIT_acm;
     }
 
     if ( arch_domain_create(d) != 0 )
-        goto fail4;
+        goto fail;
+    init_status |= INIT_arch;
 
     d->iomem_caps = rangeset_new(d, "I/O Memory", RANGESETF_prettyprint_hex);
     d->irq_caps   = rangeset_new(d, "Interrupts", 0);
     if ( (d->iomem_caps == NULL) || (d->irq_caps == NULL) )
-        goto fail5;
+        goto fail;
 
     if ( sched_init_domain(d) != 0 )
-        goto fail5;
+        goto fail;
 
     if ( !is_idle_domain(d) )
     {
@@ -224,10 +230,6 @@ struct domain *domain_create(
                 break;
         d->next_in_list = *pd;
         d->next_in_hashbucket = domain_hash[DOMAIN_HASH(domid)];
-        /* Two rcu assignments are not atomic 
-         * Readers may see inconsistent domlist and hash table
-         * That is OK as long as each RCU reader-side critical section uses
-         * only one or them  */
         rcu_assign_pointer(*pd, d);
         rcu_assign_pointer(domain_hash[DOMAIN_HASH(domid)], d);
         spin_unlock(&domlist_update_lock);
@@ -235,18 +237,17 @@ struct domain *domain_create(
 
     return d;
 
- fail5:
-    arch_domain_destroy(d);
- fail4:
-    if ( !is_idle_domain(d) )
+ fail:
+    d->is_dying = 1;
+    atomic_set(&d->refcnt, DOMAIN_DESTROYED);
+    if ( init_status & INIT_arch )
+        arch_domain_destroy(d);
+    if ( init_status & INIT_acm )
         acm_domain_destroy(d);
- fail3:
-    if ( !is_idle_domain(d) )
+    if ( init_status & INIT_gnttab )
         grant_table_destroy(d);
- fail2:
-    if ( !is_idle_domain(d) )
+    if ( init_status & INIT_evtchn )
         evtchn_destroy(d);
- fail1:
     rangeset_domain_destroy(d);
     free_domain(d);
     return NULL;
@@ -308,6 +309,7 @@ void domain_kill(struct domain *d)
         return;
     }
 
+    evtchn_destroy(d);
     gnttab_release_mappings(d);
     domain_relinquish_resources(d);
     put_domain(d);
@@ -473,7 +475,6 @@ static void complete_domain_destroy(struct rcu_head *head)
 
     rangeset_domain_destroy(d);
 
-    evtchn_destroy(d);
     grant_table_destroy(d);
 
     arch_domain_destroy(d);

diff --git a/xen/common/event_channel.c b/xen/common/event_channel.c
index c809c548b6c48cd5835ae9f606b5e869af6208c1..09533a0e14d6bf2091941b38edf7aabcc4291e98 100644 (file)
--- a/xen/common/event_channel.c
+++ b/xen/common/event_channel.c
@@ -79,6 +79,9 @@ static int get_free_port(struct domain *d)
     struct evtchn *chn;
     int            port;
 
+    if ( d->is_dying )
+        return -EINVAL;
+
     for ( port = 0; port_is_valid(d, port); port++ )
         if ( evtchn_from_port(d, port)->state == ECS_FREE )
             return port;
@@ -374,14 +377,7 @@ static long __evtchn_close(struct domain *d1, int port1)
 
             /* If we unlock d1 then we could lose d2. Must get a reference. */
             if ( unlikely(!get_domain(d2)) )
-            {
-                /*
-                 * Failed to obtain a reference. No matter: d2 must be dying
-                 * and so will close this event channel for us.
-                 */
-                d2 = NULL;
-                goto out;
-            }
+                BUG();
 
             if ( d1 < d2 )
             {
@@ -403,7 +399,6 @@ static long __evtchn_close(struct domain *d1, int port1)
              * port in ECS_CLOSED. It must have passed through that state for
              * us to end up here, so it's a valid error to return.
              */
-            BUG_ON(d1 != current->domain);
             rc = -EINVAL;
             goto out;
         }
@@ -960,14 +955,22 @@ void evtchn_destroy(struct domain *d)
 {
     int i;
 
+    /* After this barrier no new event-channel allocations can occur. */
+    BUG_ON(!d->is_dying);
+    spin_barrier(&d->evtchn_lock);
+
+    /* Close all existing event channels. */
     for ( i = 0; port_is_valid(d, i); i++ )
     {
         evtchn_from_port(d, i)->consumer_is_xen = 0;
         (void)__evtchn_close(d, i);
     }
 
+    /* Free all event-channel buckets. */
+    spin_lock(&d->evtchn_lock);
     for ( i = 0; i < NR_EVTCHN_BUCKETS; i++ )
         xfree(d->evtchn[i]);
+    spin_unlock(&d->evtchn_lock);
 }
 
 /*